🤖 AI Code Review

共审查 10 个变更文件
⚠️ 另有 12 个文件超过上限（最多 10 个）未审查

📁 CONTRIBUTING.md

🔴 缺少 OptimizationPass 导入路径 — 步骤 2 说 “Subclass OptimizationPass”，但没给出它在哪个模块、如何导入。贡献者只能猜 scratchv/optimizer/ 下的具体文件。建议补一行最小示例：

from scratchv.optimizer.base import OptimizationPass

🟡 寄存器示例不明确 — 步骤 4 只写 “Register it in create_optimization_pass_manager()”，没说注册的是类还是实例，也没给 API 名称。建议补充：

manager.add(MyPass())

否则“register”容易被理解成装饰器、类继承注册或依赖注入，不同实现差异很大。

🟡 “ordered reports” 含义含糊 — 管理器中的报告按什么顺序？按 name 排序、按注册顺序、还是按执行顺序？这会影响贡献者判断自己的 pass 在报告中的位置。请明确指出。

💭 “lowercase, kebab-case” 冗余 — kebab-case 已经规定全小写，写 “kebab-case name” 即可，避免读者怀疑是否有 “upper-kebab-case” 形式。

💭 “stable” 建议补充原因 — 如果 name 会出现在日志、报告或配置文件中，建议说明“重命名会破坏历史报告/配置映射”，否则“stable”偏向主观。

📁 benchmarks/run_benchmark.py

🔴 manager.run(program) 的副作用契约需确认
旧代码明确通过 ConstantFolder(program).run() 原地修改 program。现在依赖 manager.run(program) 的返回值报告时间，但若新 pass manager 返回新的 Program 而不是原地修改，result.ir_opt_inst_count 统计的仍可能是未优化 IR。请确认 run 是原地修改，或改用返回值。

🔴 "none" 不再保证 0.0，且计时口径变了
旧代码对 "none" 直接返回 0.0；现在即使空 pipeline，manager.run(program).elapsed_seconds 也可能包含少量 overhead / 报告初始化时间，docstring 写的 “exactly 0.0 seconds” 并不可靠。
另外旧实现用 time.perf_counter() 包住 import + pass 执行，新实现只使用 pipeline 内部报告，manager 创建和 import 时间不再计入 optimize_time_s，和历史 benchmark 结果不可直接比较。

🟡 需要验证新旧 pipeline 等价
旧 "all" 是 ConstantFolder → DeadCodeEliminator → IRPeepholeOptimizer → MulAddFusion → LICM。请确认 create_optimization_pass_manager("all") 包含相同 pass 且顺序一致，否则 benchmark 测量的是另一套优化流程。

🟡 非法 level 行为变化
旧代码对未知 level 会静默执行 ConstantFolder + DeadCodeEliminator；现在很可能由工厂函数抛异常或映射到别的 pipeline。如果 optimize_level 来自用户输入，建议显式校验并给出清晰错误。

💭 建议保留 "none" 快速路径
即使新 manager 支持空 pipeline，也建议继续 if level == "none": return 0.0，避免无谓的 manager 构建，并保持 optimize_time_s 语义稳定。

📁 benchmarks/test_benchmark.py

🔴 断言可能过强 — test_optimize 中 assert 0 < inst_after <= inst_before 隐含两个假设：

• “all” 优化管线不会增加指令数。若其中包含内联、循环展开、向量化等可能扩展 IR 的 pass，这条断言会误报失败。

• 优化后 IR 不能为空。虽然对现有 ONNX benchmark 模型大概率成立，但若未来加入常量型模型/空函数，inst_after == 0 时会失败。

  建议：仅对已知保证不增指令的 pass 组合做该断言；或根据 create_optimization_pass_manager("all") 的实际语义明确该测试契约并文档化。

🟡 “basic” 管线内容未验证 — test_codegen_riscv 从显式调用 ConstantFolder + DeadCodeEliminator 改为 create_optimization_pass_manager("basic")，但测试内没有确认 basic 确实包含这两个 pass。如果 basic 定义变化或漏配，该测试会在不执行关键优化的情况下静默通过，失去原有覆盖。

建议：为该测试增加一条针对优化后 IR 的最小断言（例如非空/指令数变化），或在 basic 配置中显式测试包含的 pass 列表。

💭 魔术字符串 — "all" / "basic" 散落在测试中。如果 scratchv.compiler 提供常量或枚举，建议使用，避免拼写错误和后续改名维护成本。

💭 局部导入重复 — 两个测试都局部导入 create_optimization_pass_manager，虽不影响正确性，但多次参数化执行会重复 import。若模块无循环依赖问题，可提到文件顶部统一导入。

📁 docs/optimization_guide.md

🟡 LICM "Implementation" is a placeholder — The doc labels this as "Implementation" but _find_loops_and_hoist always returns 0 and never actually hoists instructions. Readers will copy it expecting a working pass. Mark it as "skeleton/pseudocode" or provide the real implementation.

🟡 --optimize alias wording is confusing — "--optimize remains an alias ... and still requires one of those three values" implies it previously took a value. If it previously was a bare flag, this is a breaking CLI change; if not, just say --optimize is now an alias for --opt-level and takes the same required value.

🟡 "left intact" is ambiguous — In the W3 ADD/SUB semantics, "NaN, infinity, and finite operations that overflow to infinity are left intact" could mean the constants are preserved or that the instruction stays unfolded. Clarify: "these cases are not folded; the original instruction is left unchanged."

🟡 Pass snippets omit name — The new intro says every IR pass defines a stable name, but the Peephole and LICM examples only show optimize. Add name = "peephole" / name = "licm" to the snippets, or note that the attribute is intentionally omitted for brevity.

💭 Forward-scan limitation should be stated — "The pass performs one forward scan" means folding one instruction can’t enable a fold later in the same invocation. If that’s intended for W3, document that no fixpoint/re-scan is performed.

💭 INT32 float conversion rule could be clearer — "integer-valued finite floats" is ambiguous for typed IR: does a FLOAT64 literal like 3.0 in an INT32 expression get converted, or is it treated as mixed/unsupported? Give a concrete example.

📁 docs/topics/04-IR优化器框架.md

🟡 `name` 的“接口”写法与实现示例自相矛盾  
基类给的是 `@property` + `...`，示例子类却是 `name = "constant-folding"`。  
Python 里这种覆盖能工作，但容易误导：对没有覆盖 `name` 的 pass，`ClassName.name` 会拿到 property 对象而不是字符串。建议在文档中明确“子类必须提供字符串属性”，或把基类示例改为 `@abstractmethod` / `raise NotImplementedError`。

🟡 “所有 pass 实现相同签名”表述已不准确  
现在新增了 `name` 属性，而“签名”通常指方法参数列表。建议改为“所有 pass 实现相同接口”。

🟡 `--opt-level` 与 `--optimize` 同时传入时优先级未定义  
既然 `--optimize` 是别名，建议写明行为，例如“同一参数组，后出现者覆盖前者”，否则 `--opt-level basic --optimize all` 会产生歧义。

🟡 工厂参数与 CLI 级别未完全对应  
示例只展示了 `create_optimization_pass_manager("basic")`，但 CLI 支持 `none|basic|all`。建议补充 factory 接受的合法值，以及每个级别对应哪些 pass，避免用户猜测。

💭 “裸 `--optimize` 不合法”偏口语化  
建议改成“`--optimize` 必须带参数值”，最好给一个 argparse 下的预期报错示例。

📁 docs/topics/archive/optimizer_framework.md

🔴 文档维护矛盾 — 文件已声明“当前公共接口以 ../04-IR优化器框架.md 和实际代码为准”，却新增了整段 API 细节（工厂函数、管线映射、CLI 行为）。归档文档不应承担当前实现文档的职责，否则两处内容必然随时间漂移。建议删除新增的 from scratchv.compiler... 到 bash 示例部分，只保留“请参考新文档”的指针。

🟡 API 名称与代码一致性未验证 — create_optimization_pass_manager("all")、OptimizationReport、pass 名称（ir-peephole、muladd-fusion、licm）是否与 scratchv.compiler 实际导出一致？文档中的示例代码会被读者直接复制使用，建议用 doctest 或自动化检查确保可运行。

🟡 链接有效性 — ../04-IR优化器框架.md 依赖文件真实存在且文件名完全匹配（含中文）。若当前实际文件名不同（如 04-IR优化器框架.md 缺失），则形成死链。请确认路径。

💭 CLI 别名描述模糊 — “--optimize 只是参数名的兼容别名，仍须显式提供 none、basic 或 all；裸 --optimize 不合法” 容易让读者误以为 --optimize 是布尔开关。建议改写为：--optimize <level> 等价于 --opt-level <level>，参数 level 必填，取值同 --opt-level。

💭 归档摘要的定位 — 原文件作为“课题历史摘要”，新增内容却变成了操作手册。若确实需要记录管线演进，建议在归档说明下加一小段“2015 年管线由 X 变为 Y”的历史描述，而非粘贴当前接口签名。

📁 docs/verification.md

🟡 文档语义不清 — Note 中“--optimize 只是 --opt-level 的参数名兼容别名”和“裸 --optimize 不合法”容易让人困惑。既然是别名，应当和 --opt-level 一样需要显式提供级别。建议改写为：

--optimize 是 --opt-level 的兼容别名，同样必须提供 none、basic 或 all。例如 --optimize all 等价于 --opt-level all；单独的 --optimize 会被拒绝（不再作为布尔开关使用）。

这样既说明了等价关系，也解释了合法性边界。

🟡 确认破坏性变更 — 如果旧版 --optimize 是布尔开关，那么本次改为带参选项是 CLI 的破坏性变更。文档虽然注明“裸 --optimize 不合法”，但建议在变更说明或迁移指南中补充一句：原 --optimize 用户需显式指定级别（如 --opt-level basic），避免升级后静默失效。

💭 术语优化 — “参数名兼容别名”不常见，建议直接称“兼容别名”即可，减少阅读负担。

🟡 验证选项值 — 请确认 CLI 实际支持 --opt-level all。如果仅支持 none/basic 或类似 0/1/2，文档则引入了错误用法。建议从实际实现核对后再合入。

📁 examples/end_to_end_pipeline.py

整体看改动清晰：从手动调用 optimizer 类迁移到 pass manager，逻辑更集中。没有发现 blocker。

🟡 对 pass 名称的硬编码依赖

changes_by_name = {execution.name: execution.changes for execution in report.executions}
folded_changes = changes_by_name["constant-folding"]
eliminated_changes = changes_by_name["dead-code-elim"]

这段假设 report.executions 里一定存在 "constant-folding" 和 "dead-code-elim"。如果以后 "basic" 的 pass 改名、顺序调整，或 report 只记录实际产生变更的 pass，这里会直接 KeyError。建议把 pass 名称作为 scratchv.compiler 暴露的常量，或提供按名称查询 execution 的辅助方法；退一步也可以用 .get() 并给出可诊断的错误信息。

🟡 第二个 demo 的打印文案与实际 pass 集不一致

print("After optimization (fold + dce + peephole):")

注释已经说明 "all" 会跑五个 canonical passes，但输出仍写“fold + dce + peephole”，会让读者误以为只有三个 pass。建议改成 "After optimization (all 5 passes)" 或列出实际 pass 名，避免示例产生误导。

💭 两个 demo 的文案风格可统一
第一个 demo 输出 "N constant fold(s), M dead-code elimination(s)"，第二个输出 "N change(s) across M passes"。都可以接受，但作为示例没有逻辑问题。

💭 create_optimization_pass_manager 在两个函数里重复 import
如果该文件还有其他 demo，可以考虑提到文件顶部；不过局部 import 本身没问题，是否调整取决于项目风格。

📁 examples/llvm_optimization_pipeline.py

🔴 可能的显示 bug — changes_by_name 把 execution.changes 直接当作可打印的“数量”，但 report.total_changes 的存在暗示 changes 可能是变更记录列表。若真是列表，print(f"Constant folds: {folded_changes}") 会打印整个列表而不是数字。请确认类型；若为列表，改用 len(execution.changes)。

🟡 字典查找过于脆弱 — changes_by_name["constant-folding"] 在 pass 名称调整（或某 pass 未记录）时会直接 KeyError。建议用 next((e.changes for e in report.executions if e.name == "constant-folding"), 0)，或若 report 提供按名称查询的接口则优先使用。

🟡 “all” 分支丢失每个 pass 的明细 — 原代码能分别看到 peephole 优化数，现在只输出 total_changes。若示例目的是展示各 pass 效果，建议同时打印各 execution.name 对应的 changes，避免用户无法判断每个 pass 是否生效。

💭 魔法字符串 — "basic"、"all"、"constant-folding"、"dead-code-elim" 都是字符串字面量。如果 scratchv.compiler 提供了对应的常量或枚举，建议引用它们，降低随库升级导致示例失效的风险。

💭 局部 import — from scratchv.compiler import create_optimization_pass_manager 放在 main() 内没有问题；但如果这是新的公共 API，建议移到文件顶部与其他 import 保持一致，也便于读者发现依赖。

📁 examples/onnx_llvm_verification.py

🔴 潜在 KeyError — 第 68-69 行硬编码 "constant-folding" 和 "dead-code-elim"。如果 pass 改名、重命名或未包含在 “basic” 管线中，程序会直接崩溃。建议：

folded = changes_by_name.get("constant-folding", 0)
if folded is None:
    raise RuntimeError("Report missing 'constant-folding' pass")

或者让 report 直接提供 get_changes(pass_name) 方法。

🟡 注释与行为矛盾 — 第 62-63 行注释写 “run() optimizes program in place and returns an immutable report”，但“in place 修改”本身就是副作用，与 “immutable report” 无关。更重要的是调用方可能没意识到 program 已被修改。若该函数确实有副作用，建议注释明确说明 “This mutates the program object”，或改为返回新程序。

🟡 executions 可能包含重复名称 — 如果 report.executions 是列表，且同一 pass 可能执行多次（例如循环内多次优化），changes_by_name 字典会只保留最后一次结果。需确认 executions 是否允许重复 pass 名，若允许则按索引累加计数。

💭 变量命名 — folded_changes/eliminated_changes 看起来像是「改动列表」，但实际是数量。建议改名 fold_count、eliminated_count，提高可读性。

💭 脆弱的外部依赖 — 新代码把「pass 存在性」和「pass 返回结构」耦合在示例脚本里。示例代码本身没问题，但如果这是给用户看的模板，建议在报告 API 里提供更稳定的访问方式（如 report.summary["constant-folding"]），减少对内部 pass 名称的依赖。

⚠️ 未审查的文件

• examples/verify_with_tinyfive.py
• scratchv/compiler.py
• scratchv/main.py
• scratchv/optimizer/constant_folding.py
• scratchv/optimizer/dead_code.py
• scratchv/optimizer/licm.py
• scratchv/optimizer/muladd_fusion.py
• scratchv/optimizer/peephole.py
• scratchv/pass_interface.py
• tests/test_optimizer.py
• tests/test_optimizer_advanced.py
• tests/test_pass_manager.py